Offline Gemma, Scrollbar und API-Key Refactoring #49
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…flow refactor - Integrated offline Gemma 3n E4B it model using MediaPipe LLM Inference. - Added ModelDownloadManager for downloading and checking model storage. - Refactored API key entry flow: removed mandatory startup popup, implemented context-aware check on model selection, and made ApiKeyDialog dismissible. - Added a fading scrollbar indicator to the chat interface. - Updated AGP to 8.2.2 for MediaPipe compatibility. - Fixed exhaustive 'when' in ApiKeyDialog. - Optimized offline inference performance by caching LlmInference instance in ScreenCaptureService. - Included built and signed APK in the root directory. Co-authored-by: Android-PowerUser <88908510+Android-PowerUser@users.noreply.github.com>
Contributor
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
This PR implements offline Gemma support, scrollbar functionality, and API key flow refactoring. While the overall implementation is functional, there are several critical issues that need to be addressed before merging:
Critical Issues Found:
- Security Vulnerability: Model URL/filename mismatch in
ModelDownloadManagercould lead to downloading incorrect files - Logic Error: Offline Gemma implementation ignores image parts, breaking multimodal functionality
- Logic Error: Download dialog doesn't validate storage space before allowing downloads
- Performance Regression: Scrollbar calculations could impact performance on lower-end devices
Key Changes Reviewed:
- ✅ API key flow refactoring removes forced popup on startup
- ✅ Scrollbar implementation with fade-in/fade-out animation
- ✅ Offline model download management with storage checks
⚠️ Offline Gemma integration needs multimodal support fixes
Recommendations:
- Fix the model URL to match the intended Gemma 3n E4B model
- Add proper image handling validation for offline models
- Implement storage space validation before downloads
- Optimize scrollbar calculations for better performance
The changes show good architectural improvements, but the identified issues could cause user-facing problems and should be resolved before deployment.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
Comment on lines
12
to
13
| const val MODEL_URL = "https://storage.googleapis.com/mediapipe-models/llm/gemma-2b-it-gpu.bin" | ||
| const val MODEL_FILE_NAME = "gemma-3n-e4b-it-offline.bin" |
There was a problem hiding this comment.
🛑 Security Vulnerability: The MODEL_URL points to a different model file (gemma-2b-it-gpu.bin) than what the filename suggests (gemma-3n-e4b-it-offline.bin). This URL/filename mismatch could lead to downloading incorrect model files or potential security issues if the URL is compromised. Additionally, the placeholder URL may not be a valid download source.
Suggested change
| const val MODEL_URL = "https://storage.googleapis.com/mediapipe-models/llm/gemma-2b-it-gpu.bin" | |
| const val MODEL_FILE_NAME = "gemma-3n-e4b-it-offline.bin" | |
| const val MODEL_URL = "" | |
| const val MODEL_FILE_NAME = "gemma-3n-e4b-it-offline.bin" |
Comment on lines
711
to
755
| private fun callOfflineGemmaApi(chatHistory: List<Content>, inputContent: Content): Pair<String?, String?> { | ||
| var responseText: String? = null | ||
| var errorMessage: String? = null | ||
|
|
||
| try { | ||
| val inference = getLlmInference() ?: return Pair(null, "Offline model not found or failed to initialize. Please download it first.") | ||
|
|
||
| // Construct prompt from history and input | ||
| val promptBuilder = StringBuilder() | ||
|
|
||
| // Add history | ||
| chatHistory.forEach { content -> | ||
| val role = if (content.role == "user") "user" else "model" | ||
| content.parts.filterIsInstance<TextPart>().forEach { | ||
| promptBuilder.append("<start_of_turn>$role\n${it.text}<end_of_turn>\n") | ||
| } | ||
| } | ||
|
|
||
| // Add current input | ||
| inputContent.parts.filterIsInstance<TextPart>().forEach { | ||
| promptBuilder.append("<start_of_turn>user\n${it.text}<end_of_turn>\n<start_of_turn>model\n") | ||
| } | ||
|
|
||
| val prompt = promptBuilder.toString() | ||
| Log.d(TAG, "Offline prompt: $prompt") | ||
|
|
||
| // Use generateResponse for simplicity in this broadcast-based architecture | ||
| // but we can simulate streaming chunks if needed. | ||
| // For now, just get the full response and send it. | ||
| responseText = inference.generateResponse(prompt) | ||
|
|
||
| // Broadcast the result as a stream chunk too, so the UI updates as if it was streaming | ||
| if (responseText != null) { | ||
| val streamIntent = Intent(ACTION_AI_STREAM_UPDATE).apply { | ||
| putExtra(EXTRA_AI_STREAM_CHUNK, responseText) | ||
| } | ||
| LocalBroadcastManager.getInstance(applicationContext).sendBroadcast(streamIntent) | ||
| } | ||
| } catch (e: Exception) { | ||
| Log.e(TAG, "Offline Gemma call failed", e) | ||
| errorMessage = e.localizedMessage ?: "Offline Gemma call failed" | ||
| } | ||
|
|
||
| return Pair(responseText, errorMessage) | ||
| } |
There was a problem hiding this comment.
🛑 Logic Error: The offline Gemma implementation only processes text parts and ignores image parts from the input content and chat history. This will cause the offline model to fail when users send images, which is a core feature of this multimodal AI application.
Suggested change
| private fun callOfflineGemmaApi(chatHistory: List<Content>, inputContent: Content): Pair<String?, String?> { | |
| var responseText: String? = null | |
| var errorMessage: String? = null | |
| try { | |
| val inference = getLlmInference() ?: return Pair(null, "Offline model not found or failed to initialize. Please download it first.") | |
| // Construct prompt from history and input | |
| val promptBuilder = StringBuilder() | |
| // Add history | |
| chatHistory.forEach { content -> | |
| val role = if (content.role == "user") "user" else "model" | |
| content.parts.filterIsInstance<TextPart>().forEach { | |
| promptBuilder.append("<start_of_turn>$role\n${it.text}<end_of_turn>\n") | |
| } | |
| } | |
| // Add current input | |
| inputContent.parts.filterIsInstance<TextPart>().forEach { | |
| promptBuilder.append("<start_of_turn>user\n${it.text}<end_of_turn>\n<start_of_turn>model\n") | |
| } | |
| val prompt = promptBuilder.toString() | |
| Log.d(TAG, "Offline prompt: $prompt") | |
| // Use generateResponse for simplicity in this broadcast-based architecture | |
| // but we can simulate streaming chunks if needed. | |
| // For now, just get the full response and send it. | |
| responseText = inference.generateResponse(prompt) | |
| // Broadcast the result as a stream chunk too, so the UI updates as if it was streaming | |
| if (responseText != null) { | |
| val streamIntent = Intent(ACTION_AI_STREAM_UPDATE).apply { | |
| putExtra(EXTRA_AI_STREAM_CHUNK, responseText) | |
| } | |
| LocalBroadcastManager.getInstance(applicationContext).sendBroadcast(streamIntent) | |
| } | |
| } catch (e: Exception) { | |
| Log.e(TAG, "Offline Gemma call failed", e) | |
| errorMessage = e.localizedMessage ?: "Offline Gemma call failed" | |
| } | |
| return Pair(responseText, errorMessage) | |
| } | |
| private fun callOfflineGemmaApi(chatHistory: List<Content>, inputContent: Content): Pair<String?, String?> { | |
| var responseText: String? = null | |
| var errorMessage: String? = null | |
| try { | |
| val inference = getLlmInference() ?: return Pair(null, "Offline model not found or failed to initialize. Please download it first.") | |
| // Check if input contains images - offline model may not support multimodal input | |
| val hasImages = inputContent.parts.any { it is ImagePart } || | |
| chatHistory.any { content -> content.parts.any { it is ImagePart } } | |
| if (hasImages) { | |
| return Pair(null, "Offline model does not support image input. Please use an online model for multimodal queries.") | |
| } | |
| // Construct prompt from history and input | |
| val promptBuilder = StringBuilder() | |
| // Add history | |
| chatHistory.forEach { content -> | |
| val role = if (content.role == "user") "user" else "model" | |
| content.parts.filterIsInstance<TextPart>().forEach { | |
| promptBuilder.append("<start_of_turn>$role\n${it.text}<end_of_turn>\n") | |
| } | |
| } | |
| // Add current input | |
| inputContent.parts.filterIsInstance<TextPart>().forEach { | |
| promptBuilder.append("<start_of_turn>user\n${it.text}<end_of_turn>\n<start_of_turn>model\n") | |
| } | |
| val prompt = promptBuilder.toString() | |
| Log.d(TAG, "Offline prompt: $prompt") | |
| // Use generateResponse for simplicity in this broadcast-based architecture | |
| // but we can simulate streaming chunks if needed. | |
| // For now, just get the full response and send it. | |
| responseText = inference.generateResponse(prompt) | |
| // Broadcast the result as a stream chunk too, so the UI updates as if it was streaming | |
| if (responseText != null) { | |
| val streamIntent = Intent(ACTION_AI_STREAM_UPDATE).apply { | |
| putExtra(EXTRA_AI_STREAM_CHUNK, responseText) | |
| } | |
| LocalBroadcastManager.getInstance(applicationContext).sendBroadcast(streamIntent) | |
| } | |
| } catch (e: Exception) { | |
| Log.e(TAG, "Offline Gemma call failed", e) | |
| errorMessage = e.localizedMessage ?: "Offline Gemma call failed" | |
| } | |
| return Pair(responseText, errorMessage) | |
| } |
Comment on lines
+342
to
+349
| confirmButton = { | ||
| TextButton( | ||
| onClick = { | ||
| ModelDownloadManager.downloadModel(context) | ||
| showDownloadDialog = false | ||
| Toast.makeText(context, "Download started...", Toast.LENGTH_SHORT).show() | ||
| } | ||
| ) { Text("OK") } |
There was a problem hiding this comment.
🛑 Logic Error: The download dialog doesn't check if there's sufficient storage space before allowing the download. Users could attempt to download a 4.7 GB model without enough space, leading to failed downloads and poor user experience.
Suggested change
| confirmButton = { | |
| TextButton( | |
| onClick = { | |
| ModelDownloadManager.downloadModel(context) | |
| showDownloadDialog = false | |
| Toast.makeText(context, "Download started...", Toast.LENGTH_SHORT).show() | |
| } | |
| ) { Text("OK") } | |
| confirmButton = { | |
| TextButton( | |
| onClick = { | |
| val availableSpace = ModelDownloadManager.getAvailableStorageGB(context) | |
| if (availableSpace < 5.0) { // Need at least 5GB for 4.7GB model + buffer | |
| Toast.makeText(context, "Insufficient storage space. Need at least 5 GB available.", Toast.LENGTH_LONG).show() | |
| } else { | |
| ModelDownloadManager.downloadModel(context) | |
| showDownloadDialog = false | |
| Toast.makeText(context, "Download started...", Toast.LENGTH_SHORT).show() | |
| } | |
| } | |
| ) { Text("OK") } |
Comment on lines
+444
to
+478
| .drawWithContent { | ||
| drawContent() | ||
| if (scrollbarAlpha > 0f) { | ||
| val layoutInfo = listState.layoutInfo | ||
| val visibleItemsInfo = layoutInfo.visibleItemsInfo | ||
| if (visibleItemsInfo.isNotEmpty()) { | ||
| val totalItemsCount = layoutInfo.totalItemsCount | ||
| val viewportHeight = size.height | ||
|
|
||
| val firstVisibleItem = visibleItemsInfo.first() | ||
| val lastVisibleItem = visibleItemsInfo.last() | ||
|
|
||
| val visibleItemsCount = visibleItemsInfo.size | ||
| val scrollbarHeight = (visibleItemsCount.toFloat() / totalItemsCount) * viewportHeight | ||
|
|
||
| val firstItemIndex = firstVisibleItem.index | ||
| val firstItemOffset = listState.firstVisibleItemScrollOffset.toFloat() | ||
| val firstItemHeight = firstVisibleItem.size.toFloat() | ||
|
|
||
| val scrollPercentage = if (totalItemsCount > visibleItemsCount) { | ||
| (firstItemIndex + firstItemOffset / firstItemHeight) / totalItemsCount | ||
| } else { | ||
| 0f | ||
| } | ||
|
|
||
| val scrollbarY = scrollPercentage * viewportHeight | ||
|
|
||
| drawRect( | ||
| color = Color.Gray.copy(alpha = scrollbarAlpha), | ||
| topLeft = Offset(size.width - 4.dp.toPx(), scrollbarY), | ||
| size = Size(4.dp.toPx(), scrollbarHeight.coerceAtLeast(20.dp.toPx())) | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛑 Performance Regression: The scrollbar implementation performs complex calculations on every frame during scrolling, which could cause performance issues on lower-end devices. The calculations include division operations and multiple property accesses that execute repeatedly during scroll events.
Suggested change
| .drawWithContent { | |
| drawContent() | |
| if (scrollbarAlpha > 0f) { | |
| val layoutInfo = listState.layoutInfo | |
| val visibleItemsInfo = layoutInfo.visibleItemsInfo | |
| if (visibleItemsInfo.isNotEmpty()) { | |
| val totalItemsCount = layoutInfo.totalItemsCount | |
| val viewportHeight = size.height | |
| val firstVisibleItem = visibleItemsInfo.first() | |
| val lastVisibleItem = visibleItemsInfo.last() | |
| val visibleItemsCount = visibleItemsInfo.size | |
| val scrollbarHeight = (visibleItemsCount.toFloat() / totalItemsCount) * viewportHeight | |
| val firstItemIndex = firstVisibleItem.index | |
| val firstItemOffset = listState.firstVisibleItemScrollOffset.toFloat() | |
| val firstItemHeight = firstVisibleItem.size.toFloat() | |
| val scrollPercentage = if (totalItemsCount > visibleItemsCount) { | |
| (firstItemIndex + firstItemOffset / firstItemHeight) / totalItemsCount | |
| } else { | |
| 0f | |
| } | |
| val scrollbarY = scrollPercentage * viewportHeight | |
| drawRect( | |
| color = Color.Gray.copy(alpha = scrollbarAlpha), | |
| topLeft = Offset(size.width - 4.dp.toPx(), scrollbarY), | |
| size = Size(4.dp.toPx(), scrollbarHeight.coerceAtLeast(20.dp.toPx())) | |
| ) | |
| } | |
| } | |
| } | |
| .drawWithContent { | |
| drawContent() | |
| if (scrollbarAlpha > 0f) { | |
| val layoutInfo = listState.layoutInfo | |
| val visibleItemsInfo = layoutInfo.visibleItemsInfo | |
| if (visibleItemsInfo.isNotEmpty() && layoutInfo.totalItemsCount > 0) { | |
| val totalItemsCount = layoutInfo.totalItemsCount | |
| val viewportHeight = size.height | |
| // Early exit for edge cases | |
| if (viewportHeight <= 0 || totalItemsCount <= 1) return@drawWithContent | |
| val firstVisibleItem = visibleItemsInfo.first() | |
| val visibleItemsCount = visibleItemsInfo.size | |
| // Cache calculations to avoid repeated operations | |
| val scrollbarHeight = (visibleItemsCount.toFloat() / totalItemsCount) * viewportHeight | |
| val firstItemIndex = firstVisibleItem.index | |
| val firstItemOffset = listState.firstVisibleItemScrollOffset.toFloat() | |
| val firstItemHeight = firstVisibleItem.size.toFloat() | |
| val scrollPercentage = if (totalItemsCount > visibleItemsCount && firstItemHeight > 0) { | |
| (firstItemIndex + firstItemOffset / firstItemHeight) / totalItemsCount | |
| } else { | |
| 0f | |
| } | |
| val scrollbarY = scrollPercentage * viewportHeight | |
| val minScrollbarHeight = 20.dp.toPx() | |
| drawRect( | |
| color = Color.Gray.copy(alpha = scrollbarAlpha), | |
| topLeft = Offset(size.width - 4.dp.toPx(), scrollbarY), | |
| size = Size(4.dp.toPx(), scrollbarHeight.coerceAtLeast(minScrollbarHeight)) | |
| ) | |
| } | |
| } | |
| } |
…mprove API flow - Updated ModelDownloadManager with correct Gemma 3 4B it (Multimodal) URL and file name. - Modified MenuScreen to prompt for model download immediately upon selection in the dropdown. - Improved ScreenCaptureService to handle screenshots for offline models (simulating multimodal prompt). - Clarified build tools update (AGP 8.2.2) for MediaPipe 0.10.27 compatibility. - Fixed API key entry flow to ensure provider pre-selection and dismissibility. - Added fading scrollbar to chat UI. - Included updated signed APK. Co-authored-by: Android-PowerUser <88908510+Android-PowerUser@users.noreply.github.com>
…nload URL - Implemented LlmInferenceSession in ScreenCaptureService for multimodal (image + text) offline inference. - Corrected the Gemma 3n 4B it download URL and file name. - Ensured the download dialog is shown immediately upon model selection. - Fixed GraphOptions imports for MediaPipe tasks-genai. - Cached LlmInference and LlmInferenceSession for optimal performance. - Updated signed APK. Co-authored-by: Android-PowerUser <88908510+Android-PowerUser@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ich habe die gewünschten Änderungen implementiert:
Offline Gemma 3n E4B it (GPU):
ScreenCaptureService) implementiert, um die GPU-Nutzung und Performance zu optimieren (Instanz-Caching).API-Key Flow Refactoring:
ApiKeyDialogist nun wegklickbar. Wenn er ohne Key weggeklickt wird, bleibt die App auf der Startseite.Chat Scrollbar:
Zusätzlich wurde das Android Gradle Plugin auf Version 8.2.2 aktualisiert, um Kompatibilität mit MediaPipe sicherzustellen. Die signierte APK (
app-release-signed.apk) befindet sich im Stammverzeichnis.PR created automatically by Jules for task 18218192389146845628 started by @Android-PowerUser